Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test and fix for #154 #155

Merged
merged 2 commits into from
Jun 21, 2019
Merged

test and fix for #154 #155

merged 2 commits into from
Jun 21, 2019

Conversation

ondras
Copy link
Contributor

@ondras ondras commented Jan 15, 2019

This commit introduces a trivial fix for #154. All line segments are now tested. A unit test is included as well.

@Fil
Copy link
Member

Fil commented Jan 15, 2019

Thank you for the fix! (I'm the culprit here.)

I think we can make a few more changes:

  1. Optimize by not computing the same distance twice for each point.

  2. In the tests, it would be nice to add points that do not belong to the line's coordinates (some of which should be contained, some which shouldn't).

  3. Finally, seeing Mike's drawing and thinking again about the formula:

It appears that, when the point tested is close to a vertex, the value A = ao+bo-ab is close to the distance from the point to the line (d), but if we are "in between" it is more like the square of that distance (A ~= d**2) — so our current test is very laxist and will say that a point belongs to a line segment if its distance is less than ~sqrt(epsilon), i.e. 1e-3.

I don't want to derive the exact formula in spherical coordinates, but, if I'm not mistaken, a better approximation of the distance would be

d**2 ~= (ao + bo - ab) * (ab**2 - (ao - bo)**2) / ab

meaning that, if we want to check that "O is between A and B" and "d < ~epsilon", we should test with

(ao + bo - ab) * (ab**2 - (ao - bo) ** 2) < epsilon2 * ab

(and make sure that ao <= ab and bo <= ab)

I've set up a notebook here to explore the formula
https://beta.observablehq.com/d/39e5f445a82cbff8

@ondras
Copy link
Contributor Author

ondras commented Jan 16, 2019

Hi @Fil ,

thanks for your comments!

1. Optimize by not computing the same distance twice for each point.

Makes sense. I will try to refactor, although that would break the current clean functional containsLineSegment separation :)

2. In the tests, it would be nice to add points that do not belong to the line's coordinates (some of which should be contained, some which shouldn't).

Why not. However, this also holds for other tests for this D3 module. Perhaps a separate issue/PR shall be open for this?

3. Finally, seeing Mike's drawing and thinking again about the formula:

I provided my own (simpler, probably less correct) approximation in #153 (comment); I'm going to fork your observable notebook to see how my formula turns out.

In the tests, add points that do not belong to the line's coordinates, and some that do.

A review of the formula based on https://beta.observablehq.com/d/39e5f445a82cbff8
@Fil
Copy link
Member

Fil commented Jan 25, 2019

I have just added a few changes as per my previous comment. Would you like to review them? The containsLineSegment was nice but I feel it's better to avoid doubling the number of distance computations.

@Fil Fil requested a review from mbostock January 25, 2019 11:33
@Fil Fil merged commit 89d31fa into d3:master Jun 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants